-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Bulk Insert: Single Column and MySQL earlier upsert syntax #734
Conversation
Pull Request Test Coverage Report for Build 240
💛 - Coveralls |
This solves issues I was having with the new batch insert feature, which was generating statements with unbalanced parentheses, eg Many thanks @QuangTung97 |
Thanks @QuangTung97! By the way, this should also fix some additional cases:
Could you add those test cases as well, please? Here's the gist of the ones I added: https://gist.github.com/w1ck3dg0ph3r/cfbce1590fdce1ce54a179b6cb74f6a4. |
Thanks. I added test for the multiple lines query case. But the case of function calls might be redundant with the test case |
This looks great; this regex has seen a lot of authors and is difficult to get right, but this looks great and more tests are also welcome. Going to cut a new dot release w/ this sometime this weekend, triaging more in the backlog. |
@@ -224,29 +224,47 @@ func bindStruct(bindType int, query string, arg interface{}, m *reflectx.Mapper) | |||
return bound, arglist, nil | |||
} | |||
|
|||
var valueBracketReg = regexp.MustCompile(`(?i)VALUES\s*(\([^(]*.[^(]\))`) | |||
var valuesReg = regexp.MustCompile(`\)\s*(?i)VALUES\s*\(`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also care for inserts without an explicit column list here, e.g. insert into tblname values (...)
? Will some cases break, If we're not looking for closing bracket before "values"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Thanks for pointing out. But I haven't found a good way to both satisfying that case and the case when the name of table containing suffix: "values".
Any idea?
But because it's usually a bad idea to do that, so might be simple as documenting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think \W+(?i)VALUES\s*\(
should handle most cases, including:
INSERT INTO foo_values (a,b,c,d) VALUES (?, ?, ?, ?)
INSERT INTO foo_values VALUES (?, ?, ?, ?)
INSERT INTO `values`VALUES (?, ?, ?, ?)
While generally being a bad idea, I have no doubt that somebody does implement it, given such a wide adoption of sqlx)
This change handle the case of using single column in "VALUES(...)".
And allow using VALUES in "INSERT ... ON DUPLICATE KEY UPDATE ..." kind of queries, e.g issue #722
Possible fixes for #701 #695 #694